Close the Zenoh session after destroying the last node #21
Conversation
| // Erase the node from the set of session_nodes and close the Zenoh | ||
| // session if this is the last node. | ||
| node->context->impl->session_nodes_.erase(node); | ||
| if (node->context->impl->session_nodes_.empty()) { |
There was a problem hiding this comment.
Shouldn't you also test if node->context->impl->is_shutdown == true ?
I mean could it happen that some process close all its Nodes without calling rcl_shutdown(), but re-creating Nodes later ?
👆My bad: on a graceful closure, the Nodes are deleted before calling rcl_shutdown(), and is_shutdown is always false on last node removal.
|
Could it happen that some ROS application do the following steps:
In this case the Session is closed at step 2, and step 3 will fail. |
Not that i'm aware of. But there could be some tests that aim to do this.... After thinking about this further, I think the best way forward that works with all these different scenarios is to be able to close a session within |
Signed-off-by: Yadunund <[email protected]>
|
Actually after looking at the So i'm inclined towards closing this PR and fixing the test expectations. |
|
I'm also in favor of closing the session in It would also have the beneficial side effect to lower the traffic when a process shutdowns without explicitly cleaning up its Node and pub/sub/services. At closure Zenoh won't send all the The drawback is that the order of tokens unregistration is not guaranteed in this case, and the Node's token will probably be unregistered before some of the Node's pub/sub/services tokens. But I think those logs can be set to |
|
Okay to change the log levels to |
With this change I can get the failing test here to pass. Didn't bother with thread safety here since that is the focus of other PRs in upstream rmw_zenoh.